fix(CalendarMonth): does not skip a month on arrow click #9722

adamviktora · 2023-10-09T13:18:31Z

What: Closes #9700

The bug was related to incorrect setting of initialDate, if dateProp was null, new Date(null) returns a valid date: 1 st Jan 1970, but in countries with UTC minus it returns 31 Dec 1969, then if the initial day is 31 it was causing issues in changeMonth function, which is now also refactored and fixed.

Other changes made:

changeMonth refactoring (as we are not focusing dates on month change, changeMonth was simplified to always return the first day of the month)
change year functionality refactoring
unfocusing date, when changing year by clicking on small arrows

* now focusing date on month change * change month and change year refactoring

patternfly-build · 2023-10-09T13:27:34Z

Preview: https://patternfly-react-pr-9722.surge.sh

A11y report: https://patternfly-react-pr-9722-a11y.surge.sh

thatblindgeye

Looks like the original bug is resolved, but one thing I'm not sure about is always focusing the 1st day of the month. I'd at least expect the existing behavior where if the currently displayed month has a selected date, then that selected date receives focus when tabbing through the component.

Seems like there are a variety of ways to handle which date is focused, though. Always focusing the 1st like this PR does, MUI's datepicker looks to focus the 1st when going forward and the last date when going backward, while Material's datepicker seems to function similar to our existing behavior of the selected/previously focused date receiving focus when changing months.

@andrew-ronaldson @tlabaj wdyt?

thatblindgeye · 2023-10-10T13:53:43Z

packages/react-core/src/components/CalendarMonth/CalendarMonth.tsx

                  setFocusedDate(newDate);
                  setHoveredDate(newDate);
                  setShouldFocus(false);
+                  focusRef.current.blur(); // will unfocus a date when changing year via up/down arrows


This shouldn't be necessary. Clicking the input arrows for the year will place focus on the input itself

It looks like google calendar does not focus a date as scrolling through months. Adam had pinged me about htis yesterday and the PF 5.0 version did nit focus a specific month.

adamviktora · 2023-10-11T09:32:18Z

@thatblindgeye

If we want to keep the functionality of keeping focus on the selected date, when tabbing through months / years, we can go two commits back. It will also highlight the focused date with the lightblue circle.

Screen.Recording.2023-10-11.at.10.51.42.mov

If we want to keep the functionality of keeping focus on the selected date, but without highlighting with the lightblue circle, we can go one commit back. (same as the Material's datepicker, which actually has a bug there, if we select 31st then tab through shorter month like February, it will then focus the 28th on the upcoming months too)

Screen.Recording.2023-10-11.at.10.55.04.mov

We can keep focusing the 1st day without highlighting, Google Calendar does it this way.

I personally lean towards either option 1 or option 3 (although what we have now on patternfly.org is option 2), I think it may be confusing to not show the highlighted date and then focus the previously selected day on tabbing.

About this line:
focusRef.current.blur(); // will unfocus a date when changing year via up/down arrows
I think it is necessary, as we have to click / tab into the input first to focus it.

When running the code without that line, it keeps the focus at place. Because we do setShouldFocus(false), the condition in this useEffect will be false, so we don't execute focusRef.current.focus() (which would focus the 1st day).

  useEffect(() => {
    // Calendar month should not be focused on page load
    if ((shouldFocus || isDateFocused) && focusedDateValidated && focusRef.current) {
      focusRef.current.focus();
    }
  }, [focusedDate, isDateFocused, focusedDateValidated, focusRef]);

and just clicking the arrows does not focus the TextInput, so we don't blur the previous focus. It then also leads to these unexpected bugs shown in this video, where when I press arrow-down (in October 2021 in the video), it moves the focus from the 1st day, which is saved in focusedDate, but not actually focused:

Screen.Recording.2023-10-11.at.11.13.23.mov

thatblindgeye

@adamviktora ah okay, I'm not noticing a difference in behavior in Chrome when that line is added vs removed, but am noticing it in Firefox. Good call on that!

As far as the other focusing behavior, I don't feel super strong either way, and it's more what I might expect personally. I think what you have in this PR works really well, and it definitely simplifies the logic which is a plus, so I'd be good with the update

adamviktora · 2023-10-11T13:50:28Z

@thatblindgeye Oh, I did not realize it may be a browser thing, in Chrome the built-in input works more like I would expect.

wise-king-sullyman

Small nit/question but not something I'd block over, great work on this!

wise-king-sullyman · 2023-10-11T14:32:41Z

packages/react-core/src/components/CalendarMonth/CalendarMonth.tsx

-      return initDate;
-    } else {
-      return isValidDate(rangeStart) ? rangeStart : today;
+    if (dateProp && isValidDate(dateProp)) {


Nit: is checking that dateProp is truthy needed here? It looks like isValidDate should return false anyways if it's undefined.

Yeah, you are right, the previous issue lied in this line: const initDate = new Date(dateProp); where we create a valid date object with initial value (1st Jan 1970 / 31st Dec 1969), even if dateProp is undefined (for some reason if I console log dateProp, it prints out false and new Date(false) returns a valid date with that initial value).

So now that we check directly the dateProp, it is not needed.

anyway, my ESLint is shouting, because the isValidDate function only accepts Date and not Date | undefined, so can I keep it as it is?

You could also just change isValidDate to have the argument optional since it checks for a truthy value internally, up to you though.

wise-king-sullyman · 2023-10-11T14:32:53Z

packages/react-core/src/components/CalendarMonth/CalendarMonth.tsx

-      return isValidDate(rangeStart) ? rangeStart : today;
+    if (dateProp && isValidDate(dateProp)) {
+      return dateProp;
+    } else if (rangeStart && isValidDate(rangeStart)) {


Same question for rangeStart here.

jpuzz0 · 2023-10-18T15:29:04Z

I think this fix/change should have some unit tests backing it up. Maybe that can be a follow-up task.

adamviktora added 2 commits October 9, 2023 08:43

fix(CalendarMonth): correct setting of initialDate

8159ce5

* now focusing date on month change * change month and change year refactoring

fix(CalendarMonth): keep the dates unfocused on month change

8107be7

adamviktora requested review from thatblindgeye and tlabaj October 9, 2023 13:30

fix(CalendarMonth): unfocus date on year change

0de2966

tlabaj requested a review from jpuzz0 October 9, 2023 18:34

thatblindgeye reviewed Oct 10, 2023

View reviewed changes

thatblindgeye approved these changes Oct 11, 2023

View reviewed changes

wise-king-sullyman approved these changes Oct 12, 2023

View reviewed changes

adamviktora added 2 commits October 15, 2023 15:15

fix(CalendarMonth): undefined check

2802604

refactor(CalendarMonth): remove unnecessary undefined check

7a25128

tlabaj approved these changes Oct 18, 2023

View reviewed changes

tlabaj merged commit b09dd69 into patternfly:main Oct 18, 2023
10 checks passed

renovate bot mentioned this pull request May 1, 2024

chore(deps): update patternfly patternfly-extension-testing/react-console#26

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(CalendarMonth): does not skip a month on arrow click #9722

fix(CalendarMonth): does not skip a month on arrow click #9722

adamviktora commented Oct 9, 2023 •

edited

Loading

patternfly-build commented Oct 9, 2023 •

edited

Loading

thatblindgeye left a comment

thatblindgeye Oct 10, 2023

tlabaj Oct 10, 2023

adamviktora commented Oct 11, 2023 •

edited

Loading

thatblindgeye left a comment

adamviktora commented Oct 11, 2023

wise-king-sullyman left a comment

wise-king-sullyman Oct 11, 2023

adamviktora Oct 15, 2023 •

edited

Loading

adamviktora Oct 15, 2023 •

edited

Loading

wise-king-sullyman Oct 16, 2023

wise-king-sullyman Oct 11, 2023

jpuzz0 commented Oct 18, 2023

fix(CalendarMonth): does not skip a month on arrow click #9722

fix(CalendarMonth): does not skip a month on arrow click #9722

Conversation

adamviktora commented Oct 9, 2023 • edited Loading

patternfly-build commented Oct 9, 2023 • edited Loading

thatblindgeye left a comment

Choose a reason for hiding this comment

thatblindgeye Oct 10, 2023

Choose a reason for hiding this comment

tlabaj Oct 10, 2023

Choose a reason for hiding this comment

adamviktora commented Oct 11, 2023 • edited Loading

thatblindgeye left a comment

Choose a reason for hiding this comment

adamviktora commented Oct 11, 2023

wise-king-sullyman left a comment

Choose a reason for hiding this comment

wise-king-sullyman Oct 11, 2023

Choose a reason for hiding this comment

adamviktora Oct 15, 2023 • edited Loading

Choose a reason for hiding this comment

adamviktora Oct 15, 2023 • edited Loading

Choose a reason for hiding this comment

wise-king-sullyman Oct 16, 2023

Choose a reason for hiding this comment

wise-king-sullyman Oct 11, 2023

Choose a reason for hiding this comment

jpuzz0 commented Oct 18, 2023

adamviktora commented Oct 9, 2023 •

edited

Loading

patternfly-build commented Oct 9, 2023 •

edited

Loading

adamviktora commented Oct 11, 2023 •

edited

Loading

adamviktora Oct 15, 2023 •

edited

Loading

adamviktora Oct 15, 2023 •

edited

Loading